Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: set limits for maximum memory use. #4147

Merged
merged 8 commits into from
Aug 19, 2021

Conversation

keturn
Copy link
Member

@keturn keturn commented Sep 12, 2020

We've had some memory leaks where the game seems to take up far more of the system's memory than the amount allowed for Java's maximum heap size.

This uses some operating-system-specific functions to set limits on how much memory the process can allocate.

Because this is so system-dependent, I'm introducing this as a developer/debugger feature rather than intending it for production use.

(Players won't want the game taking runaway amounts of memory either, but getting consistent support for this and setting expectations for how it works would be a bigger project. Possibly would want to do it in Launcher, as some of these controls are better set before even launching the process.)

Contains

Linux support only.

  • Sets RLIMIT_DATA to limit the maximum amount of memory for the process's data segment.
  • Sets oom_score_adj to give the out-of-memory reaper encouragement to choose this process for termination when the system is out of memory as a whole.

How to test

On Linux:

check /proc/PID/limits

  • test default
  • test with low limit, see out-of-memory error
  • test with high limit

On other platforms:

  • make sure game starts as normal, i.e. that the Linux-only code didn't ruin things for everyone else.

Outstanding before merging

The latest in the 4.x series.
WIP: Currently hardcoded to 8 GB.
@keturn keturn added the Type: Improvement Request for or addition/enhancement of a feature label Sep 12, 2020
engine/build.gradle Outdated Show resolved Hide resolved
@keturn
Copy link
Member Author

keturn commented Sep 12, 2020

Related research note: The other mechanism Linux provides for setting these sorts of constraints is through cgroups (Control Groups). But I think setrlimit is sufficient for this purpose without getting in to cgroups.

@Cervator Cervator added Category: Performance Requests, Issues and Changes targeting performance Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. labels Sep 12, 2020
@4Denthusiast
Copy link
Contributor

If the process would otherwise exceed this limit, will it just crash, or will Java try garbage-collection?

@keturn
Copy link
Member Author

keturn commented Sep 14, 2020

If the program is allocating memory on the heap and there's not enough free space within the maximum heap size set (-Xmx setting), that's when the JVM will try garbage collection.

The process limits should be set higher than that max JVM heap size.

In testing this I've seen Java throw an OutOfMemoryError sometimes, but other times it's not been so descriptive.

It probably depends on if the thing failing to allocate the memory is Java or if it's in native code (e.g. LWJGL or OpenAL).

@keturn
Copy link
Member Author

keturn commented Oct 3, 2020

The JNA in jopenvr was removed in #4169, clearing the way for this. It also upgraded JNA so this PR can drop that part.

But since this adds command-line options, I'll say it's blocked by #4157. It's not technically blocked but I'd rather get picocli merged first and then switch this to use picocli, as opposed to making the picocli PR do more work for this one.

@keturn
Copy link
Member Author

keturn commented Aug 14, 2021

unblocked!!! now that #4157 gave us a better way to add command-line args

@keturn
Copy link
Member Author

keturn commented Aug 14, 2021

Copy link
Member Author

@keturn keturn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a few finishing touches, but it's working as expected.

Comment on lines +73 to +77
implementation(platform("org.junit:junit-bom:5.7.1")) {
// junit-bom will set version numbers for the other org.junit dependencies.
}
implementation("org.junit.jupiter:junit-jupiter-api")
implementation("org.junit.jupiter:junit-jupiter-params")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIXME: these are supposed to be test dependencies!

import java.util.regex.Matcher;
import java.util.regex.Pattern;

public class DataSizeConverter implements CommandLine.ITypeConverter<Long> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIXME: docstring

@keturn keturn marked this pull request as ready for review August 16, 2021 04:28
Copy link
Contributor

@DarkWeird DarkWeird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Works! OOM happens fast.

image

checked at:
default - works
256m - oom at splashscreen
2G - oom at game starting
8G - works

Notice strange thing:
i runs game with -Xmx1536m.. (default at runconfiguration for TerasologyPC) and game takes more memory then maximum.
I suspect this is cause of this pr.

@jdrueckert jdrueckert changed the title Set limits for maximum memory use. feat: set limits for maximum memory use. Aug 19, 2021
@jdrueckert jdrueckert merged commit a1c3ec2 into develop Aug 19, 2021
@jdrueckert jdrueckert deleted the feature/memory-limit-linux branch August 19, 2021 21:12
@keturn keturn added this to the v5.2.0 milestone Sep 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Category: Performance Requests, Issues and Changes targeting performance Type: Improvement Request for or addition/enhancement of a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants